Spec compliance bug fixes - #445
Merged
Merged
Conversation
cpu
commented
Aug 10, 2026
djc
approved these changes
Aug 11, 2026
djc
left a comment
djc
left a comment
Member
There was a problem hiding this comment.
Nice, LGTM!
I'd be in favor of squashing both commits for each fix into one.
RFC 5280 §5.3.2 defines the invalidity date CRL entry extension as InvalidityDate ::= GeneralizedTime, not the Time CHOICE used for validity and revocation dates. Using write_dt_utc_or_generalized meant any invalidity date between 1950 and 2049 was encoded with a UTCTime tag, producing an invalid extension that strict parsers reject. Write the date as GeneralizedTime unconditionally and test the results match expected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Serializing a CrlDistributionPoint with no URIs produced an empty fullName GeneralNames, violating GeneralNames ::= SEQUENCE SIZE (1..MAX) OF GeneralName (RFC 5280 §4.2.1.13). Return the new Error::EmptyCrlDistributionPointUris from certificate and CRL serialization instead of emitting invalid DER, and test the results match expected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add key_usages and crl_distribution_points to the should_write_exts gate in serialize_der_with_signer. Previously a certificate whose params requested only one of those extensions was silently issued with no extensions at all, because the gate predicted emptiness from a different set of fields than write_extensions actually writes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Filter Unspecified out of the reason code once, before both the presence check and the extension write, instead of applying the filter only in the presence check. Previously reason_code: Some(Unspecified) combined with an invalidity date wrote an ENUMERATED 0 reasonCode extension, which RFC 5280 §5.3.1 says should be absent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PKCS_RSA_PSS_SHA256 has been pub(crate) and excluded from SignatureAlgorithm::iter() (and therefore from_oid) since its introduction, so it is unreachable by users. Its encoding was also doubly wrong: it wrote saltLength 20 explicitly even though RFC 4055 declares saltLength [2] INTEGER DEFAULT 20 (the same X.690 §11.5 violation as the explicit cA FALSE bug), and the declared 20-byte salt did not match the 32-byte salt ring/aws-lc-rs actually use for RSA_PSS_SHA256 signatures. Remove the algorithm, its RsaPss params writer, the key pair loading branches, the id-RSASSA-PSS OID constant, and the commented-out references, rather than shipping latent bugs. PSS support can be reintroduced properly if there is demand. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cpu
enabled auto-merge
August 11, 2026 13:38
Member
Author
Done. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I let claude loose to do a review of bugs similar to #444 and it came up with several findings I confirmed myself. As an experiment I let it write the unit tests and fixes and then reviewed them myself to satisfaction. In summary:
The last commit drops some crate-internal RSASSA-PSS bits rather than try to fix the salt length bugs. I think that's the right call and we can revive it (correctly) in #417 when someone has the time/energy.
I opted for a "write an ignored test that catches the bug and fails without the fix, then write the fix and unignore the test" approach here because I wanted to keep the LLM on the rails. If folks prefer we can squash each of those paired commits into just one. LMK.